v2: fix transformer deadlock on Linux caused by nested lock on shared scopes#26655
v2: fix transformer deadlock on Linux caused by nested lock on shared scopes#26655medvednikov wants to merge 1 commit intomasterfrom
Conversation
… scopes Four functions in struct.v acquired `lock t.env.scopes` and then called functions (get_field_type_name, type_to_name, type_to_c_name, is_sum_type) that also try to acquire the same lock, causing a deadlock on Linux where mutexes are non-reentrant. Fix: extract the needed type data while holding the lock, then call the potentially-locking functions outside the lock block. Affected functions: - get_struct_field_type_name - resolve_struct_field_type - get_field_array_elem_sumtype_name - get_field_array_elem_c_name Also renames the transformer integration test back from _darwin_ to _nix_ since the deadlock that required restricting it to macOS is now fixed. https://claude.ai/code/session_01RQNT6SFstGf6SNVn8vT5W8
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 291569f03b
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| if field.typ is types.Array { | ||
| field_arr := field.typ as types.Array | ||
| elem_name := t.type_to_name(field_arr.elem_type) | ||
| if t.is_sum_type(elem_name) { | ||
| return elem_name | ||
| } | ||
| found_elem_type = field_arr.elem_type | ||
| found = true | ||
| } |
There was a problem hiding this comment.
get_field_array_elem_sumtype_name now keeps iterating scopes when it finds field_name but that field is not an array, because found is only set inside the types.Array branch. In projects with homonymous structs across modules, this can skip the intended struct and later pick an array field from another scope, so the transformer wraps elements as a sumtype when the target field is actually non-array. Before this refactor, the function returned immediately after the first matching field, so this introduces incorrect cross-scope resolution.
Useful? React with 👍 / 👎.
| if field.typ is types.Array { | ||
| field_arr := field.typ as types.Array | ||
| return t.type_to_c_name(field_arr.elem_type) | ||
| found_elem_type = field_arr.elem_type | ||
| found = true | ||
| } |
There was a problem hiding this comment.
get_field_array_elem_c_name has the same regression pattern: when field_name matches but the field is not an array, the function no longer returns and continues scanning other scopes. If another module has a same-named struct with an array field of that name, this can return the wrong element C type and stamp an incorrect ArrayInitExpr.typ, producing invalid transformed code for the actual target struct.
Useful? React with 👍 / 👎.
|
In general, it is always better to keep things locked for the briefest possible time, whether locks are re-entrant or not. |
| scope_names := t.env.scopes.keys() | ||
| for scope_name in scope_names { | ||
| scope := t.env.scopes[scope_name] or { continue } | ||
| // Try the lookup name directly in the appropriate module scope |
There was a problem hiding this comment.
I think these comments shouldn't be removed, as they explain what's happening 🤔
|
How is this going? I'm looking forward to the improvement of v2 |
medvednikov
force-pushed
the
master
branch
2 times, most recently
from
cdf4550 to
4dc97d9
Compare
There was a problem hiding this comment.
The comment in L1313 is not necessary. However, I agree that the L1311 one would be useful. At least it was useful for me when I was reviewing the code.
6 participants
Four functions in struct.v acquired
lock t.env.scopesand then calledfunctions (get_field_type_name, type_to_name, type_to_c_name, is_sum_type)
that also try to acquire the same lock, causing a deadlock on Linux where
mutexes are non-reentrant.
Fix: extract the needed type data while holding the lock, then call the
potentially-locking functions outside the lock block.
Affected functions:
Also renames the transformer integration test back from darwin to nix
since the deadlock that required restricting it to macOS is now fixed.
https://claude.ai/code/session_01RQNT6SFstGf6SNVn8vT5W8